Skip to content

Conversation

@vicheey
Copy link
Contributor

@vicheey vicheey commented Dec 6, 2024

Problem

Attempt to fix #6094 as the current implementation of wizard does not support:

  • async provider for showWhen
  • restoring previous state of WizardPrompter in nested wizard when customer click BACK button
  • skipping prompter in backward direction.

Solution

  • implement a new logic in NestedWizard class that create wizard classes as prompters with support for instantiating and restoring child wizards in backward direction.
  • update wizard state controller to support ControlSingal.Skip and add concept of wizard direction
  • add support for async provider for showWhen clause

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@vicheey vicheey marked this pull request as ready for review December 6, 2024 17:57
@vicheey vicheey requested a review from a team as a code owner December 6, 2024 17:57
@vicheey vicheey requested a review from seshubaws December 9, 2024 17:42
@vicheey vicheey force-pushed the add-nested-wizard-prompter-full-support branch from 53cda39 to 71ad18d Compare December 9, 2024 21:58
Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • implement a new NestedWizard abstract class that contain logic to instantiate and restore child Wizard used as prompter
  • update wizard state controller to support ControlSingal.Skip and add concept of wizard direction for tracking

Nice approach, and explanation! And clear test code.

/**
* @param val Value immediately returned by the prompter.
*/
constructor(public readonly val: T) {
Copy link
Member

@roger-zhangg roger-zhangg Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous implementation supports resolving the value val it was assigned in the beginning. but this is removed in the new solution. Is this intended?

Copy link
Contributor Author

@vicheey vicheey Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Since we have SKIP signal. Resolving value was a workaround.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would there be a case we actually want it to resolve some default value?

Copy link
Member

@roger-zhangg roger-zhangg Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A straight forward case would be in bindPrompter, we have multiple if/else based on previous states, and maybe there are two case leads to skip prompter, but we want different value for this field even though it is skipped.

With previous solution we will be able to do as the follows

        form.stuff.bindPrompter(
            (f) => {
                if (f===1) {
                     return skipPrompter(1)
                }
                if (f===2) {
                     return skipPrompter(2)
                }
               else{
               return createQuickPick
              }
            },
            { setDefault: "skipped"} // set default won't be ideal in this case
        )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous workaround for skipping prompter was to automatically resolve the value and skip displaying UI to customer. However, that did not work for backward direction since the auto resolve always revert the backward flow to forward flow.

So we are splitting the skipping case from setting default value case. For how to use the SkipPrompter, please follow this example: aws-toolkit-vscode/packages/core/src/dev/activation.ts at master · vicheey/aws-toolkit-vscode

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can consider resolving the value when forward and skip when backwards right?

Copy link
Member

@roger-zhangg roger-zhangg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this super helpful feature, few questions

/**
* Depending on current wizard direction, skip signal get converted to forward or backward control signal
*/
result.controlSignal = this.direction === DIRECTION.Forward ? undefined : ControlSignal.Back
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would this work on Wizard prompter? Will it able to identify the direction of it's parent wizard?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WizardPrompter is still considered a prompter. It is not responsible for identifying parent wizard direction.

This state management functionality belongs to the stateController object within the Wizard class. Each Wizard is responsible for prompting, receiving, and processing answers from prompters (including WizardPrompter). Each answer contains data and a ControlSignal (undefined indicates no direction change). The Wizard class's stateController tracks the current direction and uses the ControlSignal from the prompter to manage direction changes.

How do we restore the state of WizardPrompter when a back button is clicked?
Well, we do not restore the state of the WizardPrompter object, we always instantiate a new instance of this class. We, however, restore the state of Wizard class used for instantiate the WizardPrompter. Again, the (parent) wizard class is responsible for this (reference)

@justinmk3 justinmk3 merged commit c45ecfb into aws:master Dec 12, 2024
35 of 38 checks passed
* An abstract class that extends the base Wizard class plus the ability to
* use other wizard classes as prompters
*/
export abstract class NestedWizard<T> extends Wizard<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name seems confusing, unless I'm mistaken. This class is a Wizard that contains nested wizards, it's not itself nested. Right? So maybe a better name is Wizards (plural). That also improves discoverability because it sorts alphabetically next to Wizard.

@justinmk3
Copy link
Contributor

Merged to unblock. Nicely done, thanks! Some followup requests:

@vicheey vicheey deleted the add-nested-wizard-prompter-full-support branch December 13, 2024 00:37
karanA-aws pushed a commit to karanA-aws/aws-toolkit-vscode that referenced this pull request Jan 17, 2025
…aws#6166

## Problem
Wizard framework does not support:
- async `showWhen()` provider.
- restoring previous state of WizardPrompter in nested wizard when
  user clicks BACK button.
- skipping prompter in backward direction.

## Solution
- introduce `NestedWizard` class that uses child wizards as prompters with
  support for instantiating and restoring child wizards in backward direction.
- update wizard state controller to support `ControlSingal.Skip` and add concept
  of wizard direction.
- support async `showWhen()` provider.

fix aws#6094
karanA-aws pushed a commit to karanA-aws/aws-toolkit-vscode that referenced this pull request Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

arch(wizards): support async bindPrompter functions (ContextOptions)

4 participants